-
Notifications
You must be signed in to change notification settings - Fork 366
Delete TODOs we're never going to fix #3197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: 0 B Total Size: 485 kB ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (8dc0a8d) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3197If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3197If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3197 |
cfcd6de to
2d6edf0
Compare
| /** | ||
| * @deprecated - do not use in new code. | ||
| */ | ||
| getSerializedState(): State; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getSerializedState is deprecated; hence, we're never going to type this.
| showWordCount={true} | ||
| warnNoPrompt={false} | ||
| warnNoWidgets={true} | ||
| // TODO(LEMS-2656): remove TS suppression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no TS suppression here.
| // TODO(eater): The 22nd function added will be {(x) since '{' | ||
| // comes after 'z' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a funny bug, but the interaction widget is very dead, so we are never going to fix it.
| /** | ||
| * An answer item in the choices list. | ||
| * | ||
| * TODO(michaelpolyak): Implement answer reordering, CP-117 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CP-117 has been marked Won't Do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think our label-image widget editor supports re-ordering by dragging.
| // it can be triggered from within the widget, or requires | ||
| // a similar implementation locally, in which case consider | ||
| // refactoring `perseus-admin-package/image-upload-dialog.jsx` | ||
| // as common utility, CP-118 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CP-118 has been marked Won't Do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Molecule is a dead widget.
| // TODO(kevinb) actually compute the size of the graphie correctly and | ||
| // make it that size so we don't have to add extra padding. The value | ||
| // was determined by eye-balling the layout. :( | ||
| const paddingForBottomLabel = 75; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a gross hack, but plotter is deprecated and hidden from the widget dropdown in the editors, so it's probably not worth fixing.
| // be independent of everything else. | ||
| import type {KeypadKey} from "./keypad"; | ||
|
|
||
| // TODO(FEI-4010): Remove `Perseus` prefix for all types here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commentary Hm. This ticket was closed as Won't Do, but I think it's still a valuable thing. React doesn't prefix its symbols with ReactXyz (typically) and we could shorten alot of our types by removing the prefix. That said, probably a very low-value set of work.
TLDR: I'm good with removing this TODO. :)
| {value: "", content: <span>—</span>}, | ||
| {value: "->", content: <span>→</span>}, | ||
| /* | ||
| TODO(eater): fix khan-exercises so these are supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
khan-exercises?? 😂 That's a while ago
| /** | ||
| * An answer item in the choices list. | ||
| * | ||
| * TODO(michaelpolyak): Implement answer reordering, CP-117 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think our label-image widget editor supports re-ordering by dragging.
|
|
||
| /* TODO(benkomalo): ughhh. kill or move this out of here :( | ||
| Styles specifically for the mobile native apps */ | ||
| /* Styles specifically for the mobile native apps */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder ... if this is used now that mobile uses the web experience. 🤔 No change necessary on this PR, but can this be removed entirelY?
Some of these TODOs represent the wishful thinking of our forebears a decade
ago. Others have become obsolete as code changed around them. I'd like to be
able to grep for TODO in our codebase and get a sense of where the real tech
debt is, and having old, won't-fix TODOs makes that difficult.
If any of the removed TODOs are truly important, I trust they'll come back.
Issue: none
Test plan:
none